Skip to content

Test BackStopJS zaliczony#5447

Open
voyuser wants to merge 4 commits into
mate-academy:masterfrom
voyuser:develop
Open

Test BackStopJS zaliczony#5447
voyuser wants to merge 4 commits into
mate-academy:masterfrom
voyuser:develop

Conversation

@voyuser

@voyuser voyuser commented Jun 24, 2026

Copy link
Copy Markdown
  • DEMO LINK
  • TEST REPORT LINK
  • Changing 'month-lengh' and 'start-day' modifier in the code element
    reflects in changing calendar layout
  • Each day has no modifiers, only class (eg. calendar__day)
  • All Typical Mistakes from BEM lesson theory are checked.
  • Code follows all the Code Style Rules ❗️

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because a few core requirements are not yet fully met and they affect the task’s expected behavior and layout. First, the calendar width calculation ($calendar-width in src/styles/main.scss) does not correctly account for 7 day squares, the 6 gaps, and the left/right paddings using variables; this breaks the “exactly 7 columns + 10px paddings” requirement and reuses magic numbers instead of $day-squere-width, $calendar-gap-width, and a padding variable [CHECKLIST ITEM #1, #4]. Second, the month-length modifiers loop uses @for $i from 28 to 31, which excludes 31 in Sass, so no CSS rule is generated for calendar--month-length-31, meaning changing that modifier in HTML does not affect layout as required [CHECKLIST ITEM #1]. Third, the get-correction function uses ($n - 1) as a hardcoded pixel offset and ignores $calendar-gap-width and padding, so the start-day alignment is tied to implicit 1px values instead of your gap/padding variables, violating the “no repeated hardcoded px” rule and making the layout brittle [CHECKLIST ITEM #1, #4]. You should: (1) introduce a $calendar-padding variable and compute $calendar-width as 7 * $day-squere-width + 6 * $calendar-gap-width + 2 * $calendar-padding, (2) change the @for to through 31 so month-length-31 also has rules, and (3) rewrite get-correction to use a formula based on $day-squere-width and $calendar-gap-width (and padding/border if needed) instead of + $n - 1. As a minor improvement, you can also fix the $day-squere-width typo and ensure the HTML links the stylesheet that conceptually corresponds to main.scss, but these are secondary compared to the sizing and loop issues. Overall, your structure, BEM usage, and hover behavior are solid—once these calculations and the loop range are corrected, the solution should satisfy all checklist items.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/styles/index.scss
@use './main.scss';

body {
margin: 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$calendar-width currently ignores the configured gap and padding, so the calendar width will not be "exactly 7 columns + 10px paddings" as required. Update this calculation to use $day-squere-width, $calendar-gap-width, and padding values so all repeated sizes are derived from variables.

Comment thread src/styles/index.scss
Comment on lines +6 to +7
justify-content: center;
align-items: center;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get-correction uses $n - 1 directly as a pixel offset, which violates the requirement to avoid hardcoded px-like values and makes the spacing logic independent of $calendar-gap-width. Consider computing this based on $day-squere-width and $calendar-gap-width instead.

Comment thread src/styles/main.scss

$days-margin: (
'mon': 0,
'tue': get-correction(2),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires the main styles to be written in src/styles/main.scss, and the linked stylesheet should correspond to that entry; pointing to styles/index.scss instead of styles/main.scss (or the compiled CSS entry) conflicts with the description. Consider linking the same SCSS file that the description names as the main one.

Comment thread src/styles/main.scss Outdated
$day-squere-width: 100px;
$day-border-width: 1px;
$calendar-gap-width: 1px;
$calendar-width: calc($day-squere-width * 7 + 6px);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$calendar-width must represent width for 7 day squares plus gaps and paddings, and the task requires reusing variables instead of magic numbers; 6px here ignores $calendar-gap-width and padding variables and will not automatically follow if those values change.

Comment thread src/styles/main.scss Outdated
}
}

@for $i from 28 to 31 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@for $i from 28 to 31 in Sass excludes 31 because to is upper-bound exclusive; this means no calendar--month-length-31 rule is generated, violating the requirement that changing month-length (including 31) changes the layout.

Comment thread src/styles/main.scss Outdated
$calendar-width: calc($day-squere-width * 7 + 6px);

@function get-correction($n) {
@return ($n - 1) * $day-squere-width + $n - 1;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The margin calculation ($n - 1) * $day-squere-width + $n - 1 hardcodes the 1px gap ($n - 1) instead of using $calendar-gap-width, and does not factor in padding; this goes against the requirement to avoid repeated hardcoded px values and to base such calculations on variables.

Comment thread src/styles/main.scss Outdated
display: flex;
flex-wrap: wrap;
gap: $calendar-gap-width;
padding: 10px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Padding is hardcoded as 10px while the task explicitly mentions paddings in the calendar width requirement and asks to avoid repeated hardcoded values using variables; consider extracting this into a padding variable and incorporating it into $calendar-width.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there are still a few critical requirement violations that block accepting this task. First, the calendar width is not fully computed via variables: $calendar-width does not include the left/right paddings, and .calendar still uses a hardcoded padding: 10px;, which breaks the “exactly 7 columns + 10px paddings” rule and the “no repeated magic px values” rule [CHECKLIST ITEM #1, #4]. Please introduce a $calendar-padding variable, include 2 * $calendar-padding in $calendar-width, and use that same variable in the padding property. Second, the @for loop for month length uses @for $i from 28 to 31, which in Sass excludes 31, so .calendar--month-length-31 gets no rule and changing that modifier in HTML does not affect the layout as required [CHECKLIST ITEM #1]; change it to @for $i from 28 through 31. Finally, there is a SCSS lint error (operator-no-newline-after) where an expression splits a + operator across lines in src/styles/main.scss; keep the + and its right-hand operand on the same line to satisfy the linter. Once you adjust the padding/width variables, fix the @for range, and clean up the SCSS operator style, your solution will match the task requirements well—everything else (markup, hover behavior, BEM, flex layout, and day numbering) looks solid.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/index.html
<div class="calendar__day"></div>
<div class="calendar__day"></div>
<div class="calendar__day"></div>
<div class="calendar__day"></div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

padding: 10px; repeats a hardcoded px value instead of using a named variable and is not reflected in $calendar-width. This breaks the requirement to avoid repeated magic px values and to make all calculations clear with variables (checklist item #4). Consider creating $calendar-padding and using it both here and in the width formula.

Comment thread src/styles/index.scss

body {
margin: 0;
display: flex;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item #1: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout" and the description requirement "Use flex with 1px gap and limit its width to exactly 7 columns + 10px paddings". $calendar-width only accounts for day widths and gaps, but not the 10px left/right paddings, and there is no padding variable used in the width calculation.

Comment thread src/styles/index.scss
Comment on lines +7 to +8
align-items: center;
height: 100vh;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still conflicts with checklist item #1 and #4: start-day alignment must use the same variable-based layout logic (including padding/gap) and avoid implicit repeated px-based offsets. get-correction ignores padding and any border influence, so the first-day margin-left is not derived from a full calendar-width formula that includes padding.

Comment thread src/styles/main.scss Outdated
$day-border-width: 1px;
$calendar-gap-width: 1px;
$calendar-column-number: 7;
$calendar-width: $day-square-width * $calendar-column-number + ($calendar-column-number - 1) * $calendar-gap-width;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This calendar width ignores the 10px paddings below and only accounts for 7 squares and 6 gaps; also the 10px padding is a repeated magic value. This violates the requirement to have "exactly 7 columns + 10px paddings" computed via variables and breaks checklist items about changing layout via modifiers and avoiding hardcoded px used several times. Consider introducing a $calendar-padding variable and including 2 * $calendar-padding in $calendar-width, then using that variable for padding instead of a raw 10px.

Comment thread src/styles/main.scss Outdated
display: flex;
flex-wrap: wrap;
gap: $calendar-gap-width;
padding: 10px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a hardcoded 10px padding here without a variable means the actual visual width is wider than $calendar-width and repeats the same pixel value, which conflicts with the requirement to avoid hardcoded px values used multiple times and to calculate the calendar width including paddings via variables.

Comment thread src/styles/main.scss Outdated
}
}

@for $i from 28 to 31 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This @for uses from 28 to 31, which in Sass generates rules for 28, 29, and 30 only. There will be no rule for &--month-length-31, so changing the month-length-31 modifier in HTML does not affect layout as required by the task and checklist. Use through 31 to include 31 in the generated modifiers.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as an educational override so you can move forward while still learning from the remaining issues. You did a solid job on the core calendar behavior: 31 days with numbers added via ::before and @for, correct hover effects (pointer, #FFBFCB, translateY(-20px), 0.5s transition), and properly generated start-day-* and month-length-* modifiers using @each and @for. The main requirement still missing is that $calendar-width must include the left/right padding and avoid duplicated magic values: introduce a $calendar-padding variable (10px), add 2 * $calendar-padding into $calendar-width, and replace padding: 10px; in .calendar with padding: $calendar-padding; to fully satisfy [CHECKLIST ITEM #1] and the “no repeated px literals” rule. Also, there’s a stylelint error in src/styles/main.scss where a + combinator is followed by a newline (around line 5); keep the + and the next selector on the same line to fix scss/operator-no-newline-after. Finally, please remove or rephrase any personal/argumentative inline comments to keep the codebase professional and focused on the requirements. Overall, your implementation shows good command of SCSS, BEM, and layout logic—these last adjustments will bring it fully in line with the spec and typical production standards.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/index.html
<div class="calendar__day"></div>
<div class="calendar__day"></div>
<div class="calendar__day"></div>
<div class="calendar__day"></div>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still uses a hardcoded 10px instead of a padding variable included in the width calculation. It violates the requirement that the calendar width be fully computed via variables (7 columns + 10px paddings) and the previous HIGH‑priority correction to introduce $calendar-padding, include 2 * $calendar-padding in $calendar-width, and reuse that variable here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is, but it's only once in whole file, the task has nothing to do with it. It says:
" - Don't use hardcoded px values if they are used several times"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Counting Callendar width with padding is stupidity and wrong in SCSS / CSS planing..

Comment thread src/styles/main.scss Outdated
Comment on lines +21 to +26
.calendar {
width: $calendar-width;
display: flex;
flex-wrap: wrap;
gap: $calendar-gap-width;
padding: 10px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The calendar width calculation still does not include the left/right paddings, and padding: 10px; uses a hardcoded magic value. This violates the previous HIGH requirement and the generalized requirement that $calendar-width must include paddings and that padding should be driven by a $calendar-padding variable, not repeated literals (ties into checklist item #1 and the “no hardcoded px values used several times” rule).

Comment thread src/styles/main.scss Outdated
Comment on lines +75 to +77
/* Luke, ogarnij się padding liczony jako zmienna we kalkulacji gridu nie ma sensu, zadanie nie wymaga tego i to juz jest czepianie
w zadaniu jest: - Use flex with `1px` gap and limit its width to exactly 7 columns + `10px` paddings
zdecydowanie nadinterpretowałeś, reszte poprawiłem */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving a personal, argumentative comment in the source is not appropriate for the task and code style expectations. It’s better to remove this and instead align the implementation with the documented requirements (using a $calendar-padding variable and including it in $calendar-width).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants